jj git push: safety checks when pushing branch sideways, similar to git push --force-with-lease
#3522
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ilyagr
force-pushed
the
force-with-lease
branch
8 times, most recently
from
64c74f3 to
2d13e97
Compare
necauqua
reviewed
Apr 18, 2024
ilyagr
force-pushed
the
force-with-lease
branch
6 times, most recently
from
81b11aa to
f7cec4f
Compare
ilyagr
force-pushed
the
force-with-lease
branch
13 times, most recently
from
8b7e930 to
bb9547c
Compare
ilyagr
force-pushed
the
force-with-lease
branch
2 times, most recently
from
e1511cf to
f056679
Compare
|
This should be ready for review again. Apart from addressing the comments, I added a commit for adjusting |
yuja
approved these changes
May 26, 2024
ilyagr
force-pushed
the
force-with-lease
branch
5 times, most recently
from
c4d8ee4 to
eb6ba74
Compare
4 tasks
Adds two tests where pushing should fail, but currently succeeds.
This tests `git push` attempting to create a branch when the branch already unexpectedly exists on the remote. This should (and does) fail. Also changes another test to use `jj_cmd_failure`.
The new names are a bit clearer. Soem tests already use a `sideways_commit`, `parent_of_main_commit` will be needed in an upcoming test.
Hopefully, splitting the no-op portion will make the following commits easier to review.
As explained in the commit, our logic is a bit more complicated than that of `git push --force-with-lease`. This is to match the behavior of `jj git fetch` and branch conflict resolution rules.
This should be a no-op, though that is not necessarily obvious in corner cases. Note that libgit2 already performs the push negotiation even when pushing without force (without `+` in the refspec).
Now that we always force push, it should not occur in practice.
|
Thank you Yuya for the review! I plan to merge this tomorrow if I don't hear otherwise. If I hear from you earlier, I might merge it then. |
This was referenced May 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously,
jj git pushonly made sure that the branch is in the expectedlocation on the remote server when pushing a branch forward (as opposed to
sideways or backwards). Now,
jj git pushmakes a safety check in all casesand fails whenever
jj git fetchwould have introduced a conflict.In other words, previously branches that moved sideways or backward were
pushed similarly to Git's
git push --force; now they have protectionssimilar to
git push --force-with-lease(though not identical to it, to matchthe behavior of
jj git fetch). Note also that because of the wayjj git fetchworks,jjdoes not suffer from the same problems as Git'sgit push --force-with-leasein situations whengit fetchis run in the background.There are a few TODOs that may be added later (in this or separate PRs):
jj help git push, possibly elsewhere.See TODOs in the code for more details. It's possible I forgot some, but it's probably time for others to look at the general form of this PR in any case.
Checklist
If applicable:
CHANGELOG.md